Skip to content

Add event emitter abstraction#1754

Merged
charisk merged 2 commits intomainfrom
charisk/event-emitter-abstraction
Nov 11, 2022
Merged

Add event emitter abstraction#1754
charisk merged 2 commits intomainfrom
charisk/event-emitter-abstraction

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Nov 11, 2022

Currently, classes/modules that interact with events (raising or catching) have to import Event/EventEmitter which couples them to the VS Code API and as a consequence it means we're no longer able to write unit tests. It pushes us towards writing slow integration tests that require the whole extension host.

This PR introduces some custom abstractions that we can use instead of bringing in the VS Code API.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk added the secexp label Nov 11, 2022
@charisk charisk requested a review from a team as a code owner November 11, 2022 15:06
Comment thread extensions/ql-vscode/src/common/events.ts Outdated
@charisk charisk enabled auto-merge (squash) November 11, 2022 16:21
@charisk charisk merged commit a053137 into main Nov 11, 2022
@charisk charisk deleted the charisk/event-emitter-abstraction branch November 11, 2022 16:28
@adityasharad
Copy link
Copy Markdown
Contributor

DisposableObject exists for a similar reason. Should we move all these abstractions into a common location, either common/vscode or pure?

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Nov 14, 2022

DisposableObject exists for a similar reason. Should we move all these abstractions into a common location, either common/vscode or pure?

My plan/hope is to eventually move away from the term "pure" as it's a little bit confusing (makes me think of pure functions) and have /vscode in the location/name. I wanted to use this area as an example to make the case for it a bit more too, so expect to see/hear about it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants